Configure request headers used in cache keys - #800
Conversation
|
Test this PR in WordPress Playground. |
maxschmeling
changed the title
Co-authored-by: Chris Zarate <chris.zarate@automattic.com>
| class RdbCacheMiddleware extends \Kevinrob\GuzzleCache\CacheMiddleware { | ||
| public const CACHE_KEY_REQUEST_HEADERS_HEADER = 'X-Remote-Data-Blocks-Cache-Key-Headers'; | ||
|
|
||
| public function __invoke( callable $handler ): callable { |
There was a problem hiding this comment.
I'm not sure I understand what is accomplished by this __invoke override. I believe this header is already removed by the RdbCacheStrategy#getCacheObject
There was a problem hiding this comment.
Left over duplicate. I've removed it.
There was a problem hiding this comment.
The __invoke is still present
There was a problem hiding this comment.
To be clear, I don't think this __invoke has any effect. Can you test this?
There was a problem hiding this comment.
It doesn't. I removed it but didn't get that in the commit I guess. Pushed in 78741db
There was a problem hiding this comment.
This removal actually does cause a test failure.
get_cache_object() removes the header from the request stored in the cache entry, but it runs only after the downstream HTTP handler has returned. This override wraps that handler so the cache strategy can still use the metadata while the transport receives a sanitized request. Without it, the metadata header is sent to the remote API, as covered by testCacheKeyRequestHeaderMetadataIsNotSentToRequestHandler.
There was a problem hiding this comment.
I also realized this means the TTL value isn't being stripped either. So I've added a test and fix for that as well.
There was a problem hiding this comment.
Without it, the metadata header is sent to the remote API
The same is probably true of X-Remote-Data-Blocks-Cache-TTL. Is that bad?
There was a problem hiding this comment.
I'm not sure it is a bad side effect. It's just a request header. But we should pick a policy and be consistent. I'd vote to leave it personally.
There was a problem hiding this comment.
Yeah, I already removed it in the commit I commented above.
I definitely don't think we should be adding headers to requests for internal functions and letting them flow through to the endpoint. Unless you have strong opinions about it, I'm going to go with the approach of stripping them.
2 participants
Summary
cache_key_request_headersconfiguration for HTTP queriesAuthorizationandCache-Control, merging additional query header names case-insensitivelyget_cache_key_request_headers()part ofHttpQueryInterfaceand haveQueryRunnerconsume the query-owned policy directlySecurity
Remote Data Blocks does not include every request header in cache keys. If a custom header changes authentication, authorization, tenancy, or returned data, every query that uses it must list that header in
cache_key_request_headers; otherwise requests with different security contexts can share a cached response.Headers configured on a data source are not added automatically. The documentation calls this out prominently, including the cross-request and cross-user risk with persistent object caching. The built-in
AuthorizationandCache-Controldefaults cannot be removed.Compatibility
Direct implementations of
HttpQueryInterfacemust implementget_cache_key_request_headers(). Returning[]uses only the built-in defaults. The extending guide includes an upgrade example.Testing
npm run format:checknpm run lint:phpnpm run lint:jsnpm run check-typesnpm test(596 PHP tests / 1,374 assertions; 76 JavaScript tests)npm run buildgit diff --check